workflow: end a run whose host call never answers (#2748) - #2773

…flict

Second rebase by the rework line: main moved to b89ea5e when ThinkFlowLab#2773 (ThinkFlowLab#2748) merged and touched the same two files.

Conflicts, both textual:
- `docs/subsystems/workflow.md` paragraph 3: ThinkFlowLab#2748's wording is kept and this branch's drain sentence is added after "Nothing resumes it" (ThinkFlowLab#2752, ThinkFlowLab#2757).
- `plugins/workflow/src/service.js` merged without overlap: ThinkFlowLab#2748's changes sit in the deadline/serial-tail and spawn-deadline paths; this branch changes only `reap()`, which ThinkFlowLab#2748 left as it was.

The change itself is unchanged from 59a783a: `reap()` drains the lane's queue after the reaper settles, on the `hostSerial` tail every `assign` takes.

…kFlowLab#2757)

Fixes ThinkFlowLab#2752

Owner: Dev-3
Reviewer: Birfy
Written-by: Dev-3
Bot: Dev-3
Claim-Id: 360153fdbc2b4b5bbbe89b65f7dcd891
Base: f2ef4df

## The defect

A card queued behind a run that ends **failed** never started. The lane
reported `activeCount 0` / `queueDepth 1` and sat there until some later
`assign` happened to drain it — the report's `wf-3161899a` waited nine
minutes and only a re-assign moved it.

The drain lived on the finish paths (`afterStep`, the deadline, Stop,
Reset) and not on the ending the report actually hit: `host-ended`. A
host that dies leaves the run `running` in `workflows.json`; the next
load's `reap()` ends it `host-ended`, releases its step Bots, and
stopped there. That last step is a terminal transition like any other,
so the queue is what it has to move.

## What changed

- `plugins/workflow/src/service.js` — `reap()` drains the lane's queue
after the reaper settles, so a card parked behind a run this reap ended
starts on load instead of waiting for the next assign. The drain runs
**on `hostSerial`**, the tail every assign takes: the load fires the
reap and does not await it (`index.js`), so a drain outside that tail
and a concurrent `assign` both read `runningOf` before either `beginRun`
lands, each sees the same free slot and starts a card — two runs on a
lane at cap 1 (advisory FAIL on the previous tip; repro below). **Not**
in the takeover reap: that one is a detached promise started inside
`writable()`, so a drain there would fight the very `assign` that
triggered the takeover — `takeover.test.js` F2 caught that (active 2,
queue 0) while this fix was in `reapRuns`. `ThinkFlowLab#2704` keeps its rule: the
takeover reap ends the dead window's run and leaves the queue to the
next drain, which is that same assign.
- `plugins/workflow/test/queued-card-starts-after-failed-run.test.js` —
two tests. (1) two cards on one lane at `maxConcurrent 1`; the first is
left `running` in a real store, a second window loads it and calls
`reap()`: the second card's run exists and `queueDepth` is 0. (2) the
same store, but the reap is **not awaited** and an `assign` for `#C` is
called in the same tick: asserts one run at cap 1 and that the two cards
are one run plus one queued.
- `docs/subsystems/workflow.md` — the load-time reap sentence says the
ending drains the queue, on the assigns' serial tail.
- `changelog.d/2752-queued-card-after-failed-run.fixed.md` — the
user-visible entry.

## Verification at the tip

Tip SHA: `0a7d2003d5e7354ea10c5d126ef7c41ccf86ea89`

Ran:
- `cd plugins/workflow && DSH_HARNESS=… node --import ./test/resolve.mjs
--test "test/*.test.js"` → **510 tests / 510 pass / 0 fail**
- `cd core && DSH_HARNESS=… node --import ./test/harness-resolve.mjs
--test --test-concurrency=4 "test/*.test.js"` → **3134 pass / 2 fail**,
both `core/test/bench-preflight.test.js` (`bench.sh refuses before
writing the profile when pnpm is missing`, `bench.sh shows a failed pnpm
install and leaves no half-built profile`). Those two are the fixture's
own false red (`mkdir: not found` / `cat: not found` at
`bench.sh:91`/`:53`, the PATH the fixture hands the script drops
coreutils) — untouched by this change, reproduced on the clean
`f2ef4dfa` tree, and tracked as its own ticket.
- `plugins/social` 619/619, `plugins/tree` 140/140, `plugins/timeline`
161/161 (same invocation with `DSH_HARNESS` set).
- `node core/scripts/lint.mjs` → oxlint 0 errors, `arch-docs: OK`; `node
scripts/lint-plugins.mjs` over `plugins/workflow` → 0 errors.
- `node core/scripts/build-client.mjs --check` ok, `node
scripts/ds-bot-patches.mjs check` ok, `node
.github/scripts/changelog.mjs check` ok.

Not run (and why): root `npm run verify` as one command — `npm ci`
cannot reach the registry on this box (`404 tarball`, and npm cannot
write `~/.npm/_logs`), so the steps above were run individually against
a harness install.

## Red first

**(1) The reported defect**, on a clean
`f2ef4dfaccedd40315bd585474d0902118dc0ba8` with only
`plugins/workflow/test/queued-card-starts-after-failed-run.test.js`
added and the source change reverted:

```
# tests 1
# pass 0
# fail 1
not ok 1 - a card queued behind a run the host ended failed starts after the reap
  the parked card did not start after the failed run ended
  + []
  - [ '#B' ]
```

**(2) The advisory FAIL** — same store, `reap()` not awaited,
`assign('#C')` in the same tick — on the previous tip `9964ce73` (drain
present, outside `hostSerial`), source reverted to it, test present:

```
not ok 2 - a load-time reap racing an assign never starts more than the lane cap
  the lane started two cards at cap 1
# tests 2
# pass 1
# fail 1
```

Same shape on two consecutive runs in both cases, and the plugin suite
is 510/510 with the change in place. Neither `if (test)` nor a relaxed
assertion is involved: both tests drive a real store through a second
service instance and call the public `reap()` / `assign()`.

## Checkpoint

<!-- flybot:checkpoint -->
```json
{
  "pr": 2757,
  "claim": "Dev-3",
  "issue": 2752,
  "summary": "A card queued behind a failed run never started: the drain ran only on the finish paths, so a run a dead host left running, ended 'host-ended' by the load-time reap, left the queue parked with a free slot. The load-time reap now drains the queue once it settles, on the same hostSerial tail as an assign: the load does not await the reap, and outside that tail the drain and a concurrent assign each start a card at cap 1. Two tests cover it: the parking and the race.",
  "files": ["plugins/workflow/src/service.js", "plugins/workflow/test/queued-card-starts-after-failed-run.test.js", "docs/subsystems/workflow.md", "changelog.d/2752-queued-card-after-failed-run.fixed.md"],
  "checks": { "ran": "cd plugins/workflow && node --import ./test/resolve.mjs --test \"test/*.test.js\"", "result": "green", "at": "0a7d2003d5e7354ea10c5d126ef7c41ccf86ea89" },
  "next": "none — ready for review"
}
```
<!-- /flybot:checkpoint -->

## Rebased onto main (rework line)

`PUT /pulls/2757/update-branch` refused this branch twice (422 "merge
conflict between base and head"), so it was replayed by hand, keeping
main's new content:

- first onto `7b7f6c47` (ThinkFlowLab#2753/ThinkFlowLab#2750): one line of
`docs/subsystems/workflow.md`;
- then onto `b89ea5ec` (ThinkFlowLab#2773 / ThinkFlowLab#2748, which rewrote
`plugins/workflow/src/service.js`): main's ThinkFlowLab#2748 wording is kept in that
same paragraph and this branch's drain sentence is added after "Nothing
resumes it"; `service.js` did not overlap — ThinkFlowLab#2748 left `reap()` as it
was.

The change itself is unchanged; the diff is still the same 4 files,
+104/−7.

Tip is now `0a7d2003d5e7354ea10c5d126ef7c41ccf86ea89` — the Verdict must
name it.

---------

Co-authored-by: FlyBot Dev-3 <dev-3@flybot.local>

…#2782) (ThinkFlowLab#2784)

Fixes ThinkFlowLab#2782

Owner: Dev-3
Written-by: Dev-3
Bot: Dev-3
Claim-Id: 1b72bc46b1fe4eb4b877cf3b33a49ecf
Base: 3db12f0
Risk tier: T3

## The defect

Two "Always allow" answers on one Bot (or one room) could lose a grant,
and a refused one could still reach the file. `grantStanding()` and
`removeStandingApproval()` each read `target.standingApprovals`, mutated
it, and awaited `rt.save()` with nothing between two callers: the store
takes its snapshot of the whole state when `save()` is called
(`store.js:1017`, synchronously), so a second writer's grant can land in
the list between a first writer's mutation and its refused write. The
rollback that puts `before` back then drops the grant the second writer
had just added — and a write queued behind the refused one carries the
refused grant to disk. Both halves were measured on the base: memory
holding a grant `state.json` does not, and (the probe in the issue) a
refused grant written to disk.

## What changed

- `core/src/host/approvals.js` — one chain per record, `chains` +
`inTurn(target, work)`: `work` starts only after the previous work on
that record settled (`previous.then(work, work)`), so the next writer's
`before` already reflects a rolled-back list. `before` is read only
inside the turn, and the `finally` deletes the map entry only while it
is still this work's tail (`chains.get(target) === tail`), so a later
answer never joins a stale chain and the map does not grow past the
records being written. `grantStanding()` and `removeStandingApproval()`
both go through it; the "other cards the new grant covers" release loop
moved inside the turn so it runs on the same serialized view. No
behaviour change when writes do not overlap.
- `core/test/approvals-serialization.test.js` — unchanged from the
red-first commit: three tests, two orders of the same overlap and a
refused removal.
- `docs/subsystems/core-host.md` — the ThinkFlowLab#2764 sentence now states the
serialization rule.
- `changelog.d/2782-approvals-serialization.fixed.md` — the user-visible
entry.
- `patches/ds-bot/0001-core-fixes.patch` — what `generate` writes for
the new `approvals.js` body;
`patches/ds-bot/files/test/approvals-serialization.test.js` is the
mirror for the new test, which `check` requires.

## Red first

At the base with only `core/test/approvals-serialization.test.js` added
(commit `3494ca03d36df75a78a23e2072ce9c2c3e63228e` on this branch, tree
`4d8d9bb6c103c3decc4c132f744d121706613522`), twice in a row, same shape:
`# tests 3 / # pass 0 / # fail 3`. Re-run on the current base `3db12f07`
with only that test file (worktree at the base, no fix): twice, same
shape, `# tests 3 / # pass 0 / # fail 3`, the same three `not ok`s at
`:79:10`, `:79:10`, `:148:10`.

```
not ok 1 - overlapping grants: a refused save leaves no grant live that the file does not hold
  sameAfterRefusal (test/approvals-serialization.test.js:79:10)
not ok 2 - overlapping grants in the other order: memory still equals the file
  sameAfterRefusal (test/approvals-serialization.test.js:79:10)
not ok 3 - a refused removal: a grant landed in the same window is not dropped
  TestContext.<anonymous> (test/approvals-serialization.test.js:148:10)
```

1. memory `["bash:escalate:danger-full-access"]`, `state.json` `[]` —
the refused grant comes back on the other writer's stale snapshot.
2. memory `["pwsh"]`, `state.json` `[]`.
3. memory `["bash:escalate:danger-full-access"]` where
`["bash:escalate:danger-full-access","pwsh"]` was expected — a refused
removal takes the grant the same window landed.

## Verification at the tip

Tip SHA: `82bb8a7a3f6de74f586c3d55a06988775608e69d` (tree
`29f4dc6b2d43df33ef967ee8da16263ca4a993f3`), parents `628e3a07` (the fix
tip merged with `8f8aaaf2` and `b89ea5ec`) and `3db12f07` (live main,
`ThinkFlowLab#2781`).

Ran:

- `cd core && DSH_HARNESS=… node --import ./test/harness-resolve.mjs
--test test/approvals-serialization.test.js` → `# tests 3 / # pass 3 / #
fail 0`, twice in a row, same shape.
- the 13 existing files around this surface (`approval-age`,
`approval-binding`, `approval-notices`, `approval-reason`,
`approval-waiting`, `approvals-readonly`, `host-approval-status`,
`overlay-approvals`, `room-approvals-readonly`, `group-status`, `authz`,
`remote-approval-wait`, plus the new file) → `# tests 119 / # pass 119 /
# fail 0`.
- `node scripts/ds-bot-patches.mjs generate` → wrote
`patches/ds-bot/0001-core-fixes.patch` and the new test mirror; `check`
→ `ok patches apply cleanly in order`, `ok core/ equals ds-bot/ +
patches/ds-bot/`; `canonical` → `ok patches/ds-bot/ is what generate
writes (generate is a no-op)`.
- `ARCH_DOCS_BASE=3db12f07 node core/scripts/check-arch-docs.mjs` →
`arch-docs: OK (6 changed file(s))`.
- `node .github/scripts/changelog.mjs check` → `14 fragment(s) in
changelog.d/ are well formed`.
- `node .github/scripts/check-empty-catch.mjs` → exit 0.

Not run (and why): `node core/scripts/lint.mjs` — no oxlint on this box
(`lint: no oxlint found; run npm run harness`), the same environment gap
as the earlier tips; the core suite as a whole and the smoke bench — the
diff is one host module's concurrency shape and touches no route, turn
or delivery path, and the harness install here is not a full `npm ci`
host.

## Notes

- The branch was rebuilt on the pushed red-first tip (`3494ca03`, whose
tree equals the local `bb62e3cb`), and live main is merged in three
times as it moved — `8f8aaaf2` (the base this fix was first read on),
`b89ea5ec` (`ThinkFlowLab#2773`), and `3db12f07` (`ThinkFlowLab#2781`); none of them touches
`core/src/host/approvals.js`, this test, or
`patches/ds-bot/0001-core-fixes.patch`. Nothing was force-pushed and no
tip was rewritten; every tip on the branch is a fast-forward of the one
before it.
- `approvals.js` is not a gate file, so no signature is claimed here.
- The `bench/` probe from the issue is not committed.

## Checkpoint

<!-- flybot:checkpoint -->
```json
{
  "pr": 0,
  "claim": "Dev-3",
  "issue": 2782,
  "summary": "Two Always allow answers on one record overlapped: each read, mutated and saved target.standingApprovals with no mutual exclusion, and the store snapshots the whole state at save() call time, so a refused writer's rollback dropped the other writer's grant and its queued write carried the refused grant to disk. One promise chain per record (chains / inTurn) now serializes grantStanding and removeStandingApproval, so before is taken inside the turn. Three tests, red 0/3 on the base, green here.",
  "files": ["core/src/host/approvals.js", "core/test/approvals-serialization.test.js", "patches/ds-bot/0001-core-fixes.patch", "patches/ds-bot/files/test/approvals-serialization.test.js", "docs/subsystems/core-host.md", "changelog.d/2782-approvals-serialization.fixed.md"],
  "checks": { "ran": "cd core && node --import ./test/harness-resolve.mjs --test test/approvals-serialization.test.js (+13 related files); node scripts/ds-bot-patches.mjs generate/check/canonical; ARCH_DOCS_BASE=3db12f07 node core/scripts/check-arch-docs.mjs; node .github/scripts/changelog.mjs check; node .github/scripts/check-empty-catch.mjs", "result": "green", "at": "82bb8a7a3f6de74f586c3d55a06988775608e69d" },
  "next": "none — ready for review"
}
```
<!-- /flybot:checkpoint -->

---------

Co-authored-by: Dev-3 <dev-3@flybot.local>